Skip to content

Conversation

@davidBar-On
Copy link
Contributor

Suggested fix for issue #4706, including enhancements to the handling of #[rustfmt::skip].

#4706 issue was caused because format_impl() did not copy the skipped range from the local context into the visitor.context. The same issue was corrected also in format_trait() and rewrite_macro_with_items().

While testing I found that there are cases where self.line_number in push_skipped_with_span() is incorrect (usually 0 or 1). That caused an incorrect skipped range to be created. Therefore, I had to make changes to this function. The main change is that the beginning of the skipped range mainly depends on the beginning of the attributes block, instead of self.line_number.

Another major change to push_skipped_with_span() is related to the following comment:

// do not take into account the lines with attributes as part of the skipped range

This comment is not correct in general, as in several cases only the first attribute in an attributes block is not skipped (this was also partly discussed in PR #4707). Therefore, my change to push_skipped_with_span() assumes that only the first attribute line should not be skipped. If this is the correct approach, then the above comment should be removed. Otherwise, push_skipped_with_span() should be changed so the beginning of the skipped range will depend on the end of the attributes block, instead of its beginning.

I am aware that the above changes are more general than the specific #4706 issue, but there doesn't seem to be a good way to fix only this issue, because of the required change for push_skipped_with_span() to handle wrong self.line_number that impacts other skip cases.

Because of the general changes, the test cases file include tests for skipping fn/impl/trait/macro. In addition, a test was added in report.rs to make sure that the generated skip_ranges is correct.

@calebcartwright calebcartwright changed the base branch from master to rustfmt-2.0.0-rc.2 April 30, 2021 00:30
@ytmimi ytmimi added pr-targeting-2.0 This PR is targeting the 2.0 branch P-low Low priority labels Jul 27, 2022
@jieyouxu jieyouxu added the closeable??? TEMPORARY: this issue or PR probably should be closed? needs team to decide label Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

closeable??? TEMPORARY: this issue or PR probably should be closed? needs team to decide P-low Low priority pr-not-reviewed pr-targeting-2.0 This PR is targeting the 2.0 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants